Re: [Intel-gfx] [PATCH v7 1/2] drm/i915/display: Support PSR Multiple Transcoders

2020-12-22 Thread Gupta, Anshuman 


> -Original Message-
> From: Mun, Gwan-gyeong 
> Sent: Monday, December 21, 2020 3:59 PM
> To: Gupta, Anshuman 
> Cc: Nikula, Jani ; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v7 1/2] drm/i915/display: Support PSR Multiple
> Transcoders
> 
> On Wed, 2020-12-16 at 18:56 +0530, Anshuman Gupta wrote:
> > On 2020-12-16 at 14:47:42 +0200, Gwan-gyeong Mun wrote:
> > > It is a preliminary work for supporting multiple EDP PSR and
> > > DP PanelReplay. And it refactors singleton PSR to Multi Transcoder
> > > supportable PSR.
> > > And this moves and renames the i915_psr structure of
> > > drm_i915_private's to
> > > intel_dp's intel_psr structure.
> > > It also causes changes in PSR interrupt handling routine for
> > > supporting
> > > multiple transcoders. But it does not change the scenario and
> > > timing of
> > > enabling and disabling PSR. And it not support multiple pipes with
> > > a single transcoder PSR case yet.
> > >
> > > v2: Fix indentation and add comments
> > > v3: Remove Blank line
> > > v4: Rebased
> > > v5: Rebased and Addressed Anshuman's review comment.
> > > - Move calling of intel_psr_init() to intel_dp_init_connector()
> > > v6: Address Anshuman's review comments
> > >- Remove wrong comments and add comments for a limit of
> > > supporting of
> > >  a single pipe PSR
> > You missed some comment to address provided on v5.
> > Is debugfs print in drrs_status_per_crtc is not required anymore ?
> Current implementation does not enable drrs when the pipe has psr by
> intel_dp_drrs_compute_config().
> therefore the log message is not needed.
> > Also please use drm_{dbg,warn} at every place in this patch.
> I'll float a new patch which addresses it.
> > > v7: Update intel_psr_compute_config() for supporting multiple
> > > transcoder
> > > PSR on BDW+
> > Could you please send this in a separate patch, remove the PORT_A
> > restriction so that we can support multiple psr instances.
> >
> IMHO, in order to enable PSRs on multiple transcoders, we have to
> include this fix in this patch.
AFAIU keeping a separate patch would make sense, as PSR was supporting
multiple transcoders before this series with single instance but with PORT 
restriction,
 before this series PSR was used on  eDP Panel with PORT_A on different 
combination 
of pipes (transcoder).  This series supports PSR multiple instances with 
multiple transcoders.
Let me know your opinion on it.
Thanks,
Anshuman Gupta.
> > Thanks,
> > Anshuman Gupta.
> > > Signed-off-by: Gwan-gyeong Mun 
> > > Cc: José Roberto de Souza 
> > > Cc: Juha-Pekka Heikkila 
> > > Cc: Anshuman Gupta 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c  |   3 +
> > >  drivers/gpu/drm/i915/display/intel_display.c  |   4 -
> > >  .../drm/i915/display/intel_display_debugfs.c  | 111 ++--
> > >  .../drm/i915/display/intel_display_types.h|  38 ++
> > >  drivers/gpu/drm/i915/display/intel_dp.c   |  23 +-
> > >  drivers/gpu/drm/i915/display/intel_psr.c  | 588 +-
> > > 
> > >  drivers/gpu/drm/i915/display/intel_psr.h  |  14 +-
> > >  drivers/gpu/drm/i915/display/intel_sprite.c   |   6 +-
> > >  drivers/gpu/drm/i915/i915_drv.h   |  38 --
> > >  drivers/gpu/drm/i915/i915_irq.c   |  49 +-
> > >  10 files changed, 490 insertions(+), 384 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 6863236df1d0..4b87f72cb9c0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -4320,7 +4320,10 @@ static void
> intel_ddi_update_pipe_dp(struct
> > > intel_atomic_state *state,
> > >
> > >   intel_ddi_set_dp_msa(crtc_state, conn_state);
> > >
> > > + //TODO: move PSR related functions into intel_psr_update()
> > > + intel_psr2_program_trans_man_trk_ctl(intel_dp, crtc_state);
> > >   intel_psr_update(intel_dp, crtc_state, conn_state);
> > > +
> > >   intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
> > >   intel_edp_drrs_update(intel_dp, crtc_state);
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 78452de5e12f..a753647b0bcb 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -15869,8 +15869,6 @@ static void commit_pipe_config(struct
> > > intel_atomic_state *state,
> > >
> > >   if (new_crtc_state->update_pipe)
> > >   intel_pipe_fastset(old_crtc_state,
> > > new_crtc_state);
> > > -
> > > - intel_psr2_program_trans_man_trk_ctl(new_crtc_state);
> > >   }
> > >
> > >   if (dev_priv->display.atomic_update_watermarks)
> > > @@ -17829,8 +17827,6 @@ static void intel_setup_outputs(struct
> > > drm_i915_private *dev_priv)
> > >   intel_dvo_init(dev_priv);
> > >   }
> > >
> > > - intel_psr_init(dev_priv);
> > > -
> > >   for_each_i

Re: [Intel-gfx] [PATCH v7 1/2] drm/i915/display: Support PSR Multiple Transcoders

2020-12-21 Thread Mun, Gwan-gyeong 
On Wed, 2020-12-16 at 18:56 +0530, Anshuman Gupta wrote:
> On 2020-12-16 at 14:47:42 +0200, Gwan-gyeong Mun wrote:
> > It is a preliminary work for supporting multiple EDP PSR and
> > DP PanelReplay. And it refactors singleton PSR to Multi Transcoder
> > supportable PSR.
> > And this moves and renames the i915_psr structure of
> > drm_i915_private's to
> > intel_dp's intel_psr structure.
> > It also causes changes in PSR interrupt handling routine for
> > supporting
> > multiple transcoders. But it does not change the scenario and
> > timing of
> > enabling and disabling PSR. And it not support multiple pipes with
> > a single transcoder PSR case yet.
> > 
> > v2: Fix indentation and add comments
> > v3: Remove Blank line
> > v4: Rebased
> > v5: Rebased and Addressed Anshuman's review comment.
> > - Move calling of intel_psr_init() to intel_dp_init_connector()
> > v6: Address Anshuman's review comments
> >- Remove wrong comments and add comments for a limit of
> > supporting of
> >  a single pipe PSR
> You missed some comment to address provided on v5.
> Is debugfs print in drrs_status_per_crtc is not required anymore ?
Current implementation does not enable drrs when the pipe has psr by
intel_dp_drrs_compute_config().
therefore the log message is not needed.
> Also please use drm_{dbg,warn} at every place in this patch.
I'll float a new patch which addresses it.
> > v7: Update intel_psr_compute_config() for supporting multiple
> > transcoder
> > PSR on BDW+
> Could you please send this in a separate patch, remove the PORT_A 
> restriction so that we can support multiple psr instances.
> 
IMHO, in order to enable PSRs on multiple transcoders, we have to
include this fix in this patch.
> Thanks,
> Anshuman Gupta. 
> > Signed-off-by: Gwan-gyeong Mun 
> > Cc: José Roberto de Souza 
> > Cc: Juha-Pekka Heikkila 
> > Cc: Anshuman Gupta 
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c  |   3 +
> >  drivers/gpu/drm/i915/display/intel_display.c  |   4 -
> >  .../drm/i915/display/intel_display_debugfs.c  | 111 ++--
> >  .../drm/i915/display/intel_display_types.h|  38 ++
> >  drivers/gpu/drm/i915/display/intel_dp.c   |  23 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c  | 588 +-
> > 
> >  drivers/gpu/drm/i915/display/intel_psr.h  |  14 +-
> >  drivers/gpu/drm/i915/display/intel_sprite.c   |   6 +-
> >  drivers/gpu/drm/i915/i915_drv.h   |  38 --
> >  drivers/gpu/drm/i915/i915_irq.c   |  49 +-
> >  10 files changed, 490 insertions(+), 384 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 6863236df1d0..4b87f72cb9c0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -4320,7 +4320,10 @@ static void intel_ddi_update_pipe_dp(struct
> > intel_atomic_state *state,
> >  
> > intel_ddi_set_dp_msa(crtc_state, conn_state);
> >  
> > +   //TODO: move PSR related functions into intel_psr_update()
> > +   intel_psr2_program_trans_man_trk_ctl(intel_dp, crtc_state);
> > intel_psr_update(intel_dp, crtc_state, conn_state);
> > +
> > intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
> > intel_edp_drrs_update(intel_dp, crtc_state);
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 78452de5e12f..a753647b0bcb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15869,8 +15869,6 @@ static void commit_pipe_config(struct
> > intel_atomic_state *state,
> >  
> > if (new_crtc_state->update_pipe)
> > intel_pipe_fastset(old_crtc_state,
> > new_crtc_state);
> > -
> > -   intel_psr2_program_trans_man_trk_ctl(new_crtc_state);
> > }
> >  
> > if (dev_priv->display.atomic_update_watermarks)
> > @@ -17829,8 +17827,6 @@ static void intel_setup_outputs(struct
> > drm_i915_private *dev_priv)
> > intel_dvo_init(dev_priv);
> > }
> >  
> > -   intel_psr_init(dev_priv);
> > -
> > for_each_intel_encoder(&dev_priv->drm, encoder) {
> > encoder->base.possible_crtcs =
> > intel_encoder_possible_crtcs(encoder);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index cd7e5519ee7d..041053167d7f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -249,18 +249,17 @@ static int i915_psr_sink_status_show(struct
> > seq_file *m, void *data)
> > "sink internal error",
> > };
> > struct drm_connector *connector = m->private;
> > -   struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > struct intel_dp *intel_dp =
> > intel_attached_dp(to_intel_connector(conn

Re: [Intel-gfx] [PATCH v7 1/2] drm/i915/display: Support PSR Multiple Transcoders

2020-12-16 Thread Anshuman Gupta 
On 2020-12-16 at 14:47:42 +0200, Gwan-gyeong Mun wrote:
> It is a preliminary work for supporting multiple EDP PSR and
> DP PanelReplay. And it refactors singleton PSR to Multi Transcoder
> supportable PSR.
> And this moves and renames the i915_psr structure of drm_i915_private's to
> intel_dp's intel_psr structure.
> It also causes changes in PSR interrupt handling routine for supporting
> multiple transcoders. But it does not change the scenario and timing of
> enabling and disabling PSR. And it not support multiple pipes with
> a single transcoder PSR case yet.
> 
> v2: Fix indentation and add comments
> v3: Remove Blank line
> v4: Rebased
> v5: Rebased and Addressed Anshuman's review comment.
> - Move calling of intel_psr_init() to intel_dp_init_connector()
> v6: Address Anshuman's review comments
>- Remove wrong comments and add comments for a limit of supporting of
>  a single pipe PSR
You missed some comment to address provided on v5.
Is debugfs print in drrs_status_per_crtc is not required anymore ?
Also please use drm_{dbg,warn} at every place in this patch.
> v7: Update intel_psr_compute_config() for supporting multiple transcoder
> PSR on BDW+
Could you please send this in a separate patch, remove the PORT_A 
restriction so that we can support multiple psr instances.

Thanks,
Anshuman Gupta. 
> 
> Signed-off-by: Gwan-gyeong Mun 
> Cc: José Roberto de Souza 
> Cc: Juha-Pekka Heikkila 
> Cc: Anshuman Gupta 
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c  |   3 +
>  drivers/gpu/drm/i915/display/intel_display.c  |   4 -
>  .../drm/i915/display/intel_display_debugfs.c  | 111 ++--
>  .../drm/i915/display/intel_display_types.h|  38 ++
>  drivers/gpu/drm/i915/display/intel_dp.c   |  23 +-
>  drivers/gpu/drm/i915/display/intel_psr.c  | 588 +-
>  drivers/gpu/drm/i915/display/intel_psr.h  |  14 +-
>  drivers/gpu/drm/i915/display/intel_sprite.c   |   6 +-
>  drivers/gpu/drm/i915/i915_drv.h   |  38 --
>  drivers/gpu/drm/i915/i915_irq.c   |  49 +-
>  10 files changed, 490 insertions(+), 384 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 6863236df1d0..4b87f72cb9c0 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4320,7 +4320,10 @@ static void intel_ddi_update_pipe_dp(struct 
> intel_atomic_state *state,
>  
>   intel_ddi_set_dp_msa(crtc_state, conn_state);
>  
> + //TODO: move PSR related functions into intel_psr_update()
> + intel_psr2_program_trans_man_trk_ctl(intel_dp, crtc_state);
>   intel_psr_update(intel_dp, crtc_state, conn_state);
> +
>   intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
>   intel_edp_drrs_update(intel_dp, crtc_state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 78452de5e12f..a753647b0bcb 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15869,8 +15869,6 @@ static void commit_pipe_config(struct 
> intel_atomic_state *state,
>  
>   if (new_crtc_state->update_pipe)
>   intel_pipe_fastset(old_crtc_state, new_crtc_state);
> -
> - intel_psr2_program_trans_man_trk_ctl(new_crtc_state);
>   }
>  
>   if (dev_priv->display.atomic_update_watermarks)
> @@ -17829,8 +17827,6 @@ static void intel_setup_outputs(struct 
> drm_i915_private *dev_priv)
>   intel_dvo_init(dev_priv);
>   }
>  
> - intel_psr_init(dev_priv);
> -
>   for_each_intel_encoder(&dev_priv->drm, encoder) {
>   encoder->base.possible_crtcs =
>   intel_encoder_possible_crtcs(encoder);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c 
> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index cd7e5519ee7d..041053167d7f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -249,18 +249,17 @@ static int i915_psr_sink_status_show(struct seq_file 
> *m, void *data)
>   "sink internal error",
>   };
>   struct drm_connector *connector = m->private;
> - struct drm_i915_private *dev_priv = to_i915(connector->dev);
>   struct intel_dp *intel_dp =
>   intel_attached_dp(to_intel_connector(connector));
>   int ret;
>  
> - if (!CAN_PSR(dev_priv)) {
> - seq_puts(m, "PSR Unsupported\n");
> + if (connector->status != connector_status_connected)
>   return -ENODEV;
> - }
>  
> - if (connector->status != connector_status_connected)
> + if (!CAN_PSR(intel_dp)) {
> + seq_puts(m, "PSR Unsupported\n");
>   return -ENODEV;
> + }
>  
>   ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val);
>  
> @@ -280,12 +279,13 @@ stati

[Intel-gfx] [PATCH v7 1/2] drm/i915/display: Support PSR Multiple Transcoders

2020-12-16 Thread Gwan-gyeong Mun 
It is a preliminary work for supporting multiple EDP PSR and
DP PanelReplay. And it refactors singleton PSR to Multi Transcoder
supportable PSR.
And this moves and renames the i915_psr structure of drm_i915_private's to
intel_dp's intel_psr structure.
It also causes changes in PSR interrupt handling routine for supporting
multiple transcoders. But it does not change the scenario and timing of
enabling and disabling PSR. And it not support multiple pipes with
a single transcoder PSR case yet.

v2: Fix indentation and add comments
v3: Remove Blank line
v4: Rebased
v5: Rebased and Addressed Anshuman's review comment.
- Move calling of intel_psr_init() to intel_dp_init_connector()
v6: Address Anshuman's review comments
   - Remove wrong comments and add comments for a limit of supporting of
 a single pipe PSR
v7: Update intel_psr_compute_config() for supporting multiple transcoder
PSR on BDW+

Signed-off-by: Gwan-gyeong Mun 
Cc: José Roberto de Souza 
Cc: Juha-Pekka Heikkila 
Cc: Anshuman Gupta 
---
 drivers/gpu/drm/i915/display/intel_ddi.c  |   3 +
 drivers/gpu/drm/i915/display/intel_display.c  |   4 -
 .../drm/i915/display/intel_display_debugfs.c  | 111 ++--
 .../drm/i915/display/intel_display_types.h|  38 ++
 drivers/gpu/drm/i915/display/intel_dp.c   |  23 +-
 drivers/gpu/drm/i915/display/intel_psr.c  | 588 +-
 drivers/gpu/drm/i915/display/intel_psr.h  |  14 +-
 drivers/gpu/drm/i915/display/intel_sprite.c   |   6 +-
 drivers/gpu/drm/i915/i915_drv.h   |  38 --
 drivers/gpu/drm/i915/i915_irq.c   |  49 +-
 10 files changed, 490 insertions(+), 384 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
b/drivers/gpu/drm/i915/display/intel_ddi.c
index 6863236df1d0..4b87f72cb9c0 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -4320,7 +4320,10 @@ static void intel_ddi_update_pipe_dp(struct 
intel_atomic_state *state,
 
intel_ddi_set_dp_msa(crtc_state, conn_state);
 
+   //TODO: move PSR related functions into intel_psr_update()
+   intel_psr2_program_trans_man_trk_ctl(intel_dp, crtc_state);
intel_psr_update(intel_dp, crtc_state, conn_state);
+
intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
intel_edp_drrs_update(intel_dp, crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 78452de5e12f..a753647b0bcb 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15869,8 +15869,6 @@ static void commit_pipe_config(struct 
intel_atomic_state *state,
 
if (new_crtc_state->update_pipe)
intel_pipe_fastset(old_crtc_state, new_crtc_state);
-
-   intel_psr2_program_trans_man_trk_ctl(new_crtc_state);
}
 
if (dev_priv->display.atomic_update_watermarks)
@@ -17829,8 +17827,6 @@ static void intel_setup_outputs(struct drm_i915_private 
*dev_priv)
intel_dvo_init(dev_priv);
}
 
-   intel_psr_init(dev_priv);
-
for_each_intel_encoder(&dev_priv->drm, encoder) {
encoder->base.possible_crtcs =
intel_encoder_possible_crtcs(encoder);
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c 
b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index cd7e5519ee7d..041053167d7f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -249,18 +249,17 @@ static int i915_psr_sink_status_show(struct seq_file *m, 
void *data)
"sink internal error",
};
struct drm_connector *connector = m->private;
-   struct drm_i915_private *dev_priv = to_i915(connector->dev);
struct intel_dp *intel_dp =
intel_attached_dp(to_intel_connector(connector));
int ret;
 
-   if (!CAN_PSR(dev_priv)) {
-   seq_puts(m, "PSR Unsupported\n");
+   if (connector->status != connector_status_connected)
return -ENODEV;
-   }
 
-   if (connector->status != connector_status_connected)
+   if (!CAN_PSR(intel_dp)) {
+   seq_puts(m, "PSR Unsupported\n");
return -ENODEV;
+   }
 
ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val);
 
@@ -280,12 +279,13 @@ static int i915_psr_sink_status_show(struct seq_file *m, 
void *data)
 DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
 
 static void
-psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
+psr_source_status(struct intel_dp *intel_dp, struct seq_file *m)
 {
u32 val, status_val;
const char *status = "unknown";
+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
-   if (dev_priv->psr.psr2_enabled) {
+   if (intel_dp->psr.psr2_enabled) {
static const char * const live_