All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Souza, Jose" <jose.souza@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>,
	"Mun, Gwan-gyeong" <gwan-gyeong.mun@intel.com>,
	"Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v4 18/23] drm/i915/display: Introduce new intel_psr_pause/resume function
Date: Fri, 21 May 2021 21:52:06 +0000	[thread overview]
Message-ID: <c54a7e3d89987a6267b1bf997cd12039926f0282.camel@intel.com> (raw)
In-Reply-To: <559bc0e35691761220b8e709fc8319c6bf4057b9.camel@intel.com>

On Fri, 2021-05-21 at 11:58 +0100, Mun, Gwan-gyeong wrote:
> On Tue, 2021-05-18 at 14:06 +0300, Ville Syrjälä wrote:
> > On Tue, May 18, 2021 at 09:33:09AM +0000, Mun, Gwan-gyeong wrote:
> > > Hi Ville, 
> > > initially, intel_psr_pause() called intel_psr_disable_locked()
> > > instead
> > > of intel_psr_exit().
> > > In intel_psr_resume(), _intel_psr_enable_locked() was called instead
> > > of
> > > intel_psr_activate().
> > > Can you share what problem the initial code caused when calling
> > > intel_psr_pause() / intel_psr_resume()?
> > 
> > It was doing illegal stuff with crtc->state/etc. That was oopsing.
> > The other problem was that IIRC it was going to do DPCD accesses
> > while the cdclk code was already holding the aux mutexes. I moved it
> > out from under the lock, but I think we might actually want it inside
> > the lock since we'll need that to prevent PSR during all AUX transfers
> > anyway. Putting it back inside the lock should also make it less racy
> > I guess.
> > 
> > > 
> > > In addition, intel_psr_exit() /intel_psr_activate() function  disable
> > > /
> > > enable only the PSR source.
> > > So, if disable/enable for PSR Sink Device is not called together,
> > > there
> > > will be a problem that the PSR state machine of sink and source is
> > > different.
> > > What do you think?
> > 
> > If possible I wouldn't want it touch the sink at all. It should
> > basically be no different to eg. enabling the vblank interrupt.
> > 
> 
> Hi Ville and Stan, 
> Thanks, Ville, for explaining.
> 
> intel_psr_pause() and intel_psr_resume() are an api added to use when
> reactivating (disable and enable) the psr functionality without
> intel_crtc_state and drm_connector_state, as described in the commit
> log.
> And in order to deactivate and activate psr normally, we must
> deactivate the psr functionality of the sink as well, and at this time,
> sink psr deactivate using dpcd.
> 
> And in the part explaining disabling psr in cdclk setting in bspec, the
> following procedure is explained for disabling psr.
> 1. Temporarily disable PSR1, PSR2, and GTC.
> 2. Wait for disabling status from those functions.
> 3. Wait for any pending Aux transactions to complete, and do not start
> any new Aux transaction.
> ...

I don't think we need to disable, psr_exit() + wait until PSR is idle is enough, all other stuff can be left as is.

> 
> So, in my opinion, when the cdclk setting is called from
> intel_atomic_commit_tail() with functions such as
> intel_set_cdclk_pre_plane_update() /
> intel_set_cdclk_post_plane_update(),
> if psr deactivation/activation is necessary, it seems that
> intel_set_cdclk_pre_plane_update() /
> intel_set_cdclk_post_plane_update() should be called with
> intel_psr_enable() / intel_psr_disable() functions together. What do
> you think?
> 
> Br,
> G.G. 
> > > 
> > > On Mon, 2021-05-17 at 09:58 -0700, Souza, Jose wrote:
> > > > On Fri, 2021-05-14 at 20:10 -0700, Matt Roper wrote:
> > > > > From: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > > > 
> > > > > This introduces the following function that can enable and
> > > > > disable
> > > > > psr
> > > > > without intel_crtc_state/drm_connector_state when intel_psr is
> > > > > already
> > > > > enabled with current intel_crtc_state and drm_connector_state
> > > > > information.
> > > > > 
> > > > > - intel_psr_pause(): Pause current PSR. it deactivates current
> > > > > psr
> > > > > state.
> > > > > - intel_psr_resume(): Resume paused PSR without intel_crtc_state
> > > > > and
> > > > >                       drm_connector_state. It activates paused
> > > > > psr
> > > > > state.
> > > > > 
> > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > > ---
> > > > >  .../drm/i915/display/intel_display_types.h    |  1 +
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 93
> > > > > ++++++++++++++++-
> > > > > --
> > > > >  drivers/gpu/drm/i915/display/intel_psr.h      |  2 +
> > > > >  3 files changed, 82 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > index b8d1f702d808..ee7cbdd7db87 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > @@ -1482,6 +1482,7 @@ struct intel_psr {
> > > > >         bool sink_support;
> > > > >         bool source_support;
> > > > >         bool enabled;
> > > > > +       bool paused;
> > > > >         enum pipe pipe;
> > > > >         enum transcoder transcoder;
> > > > >         bool active;
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 4a63d10876ce..d4df3f5e7918 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1060,34 +1060,23 @@ static bool
> > > > > psr_interrupt_error_check(struct
> > > > > intel_dp *intel_dp)
> > > > >         return true;
> > > > >  }
> > > > >  
> > > > > -static void intel_psr_enable_locked(struct intel_dp *intel_dp,
> > > > > -                                   const struct intel_crtc_state
> > > > > *crtc_state,
> > > > > -                                   const struct
> > > > > drm_connector_state
> > > > > *conn_state)
> > > > > +static void _intel_psr_enable_locked(struct intel_dp *intel_dp,
> > > > > +                                    const struct
> > > > > intel_crtc_state
> > > > > *crtc_state)
> > > > >  {
> > > > >         struct intel_digital_port *dig_port =
> > > > > dp_to_dig_port(intel_dp);
> > > > >         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > >         struct intel_encoder *encoder = &dig_port->base;
> > > > > -       u32 val;
> > > > >  
> > > > >         drm_WARN_ON(&dev_priv->drm, intel_dp->psr.enabled);
> > > > >  
> > > > > -       intel_dp->psr.psr2_enabled = crtc_state->has_psr2;
> > > > >         intel_dp->psr.busy_frontbuffer_bits = 0;
> > > > > -       intel_dp->psr.pipe = to_intel_crtc(crtc_state-
> > > > > > uapi.crtc)-
> > > > > > pipe;
> > > > > -       intel_dp->psr.transcoder = crtc_state->cpu_transcoder;
> > > > > -       /* DC5/DC6 requires at least 6 idle frames */
> > > > > -       val =
> > > > > usecs_to_jiffies(intel_get_frame_time_us(crtc_state) *
> > > > > 6);
> > > > > -       intel_dp->psr.dc3co_exit_delay = val;
> > > > > -       intel_dp->psr.dc3co_exitline = crtc_state-
> > > > > > dc3co_exitline;
> > > > > -       intel_dp->psr.psr2_sel_fetch_enabled = crtc_state-
> > > > > > enable_psr2_sel_fetch;
> > > > >  
> > > > >         if (!psr_interrupt_error_check(intel_dp))
> > > > >                 return;
> > > > >  
> > > > >         drm_dbg_kms(&dev_priv->drm, "Enabling PSR%s\n",
> > > > >                     intel_dp->psr.psr2_enabled ? "2" : "1");
> > > > > -       intel_dp_compute_psr_vsc_sdp(intel_dp, crtc_state,
> > > > > conn_state,
> > > > > -                                    &intel_dp->psr.vsc);
> > > > > +
> > > > >         intel_write_dp_vsc_sdp(encoder, crtc_state, &intel_dp-
> > > > > > psr.vsc);
> > > > >         intel_psr_enable_sink(intel_dp);
> > > > >         intel_psr_enable_source(intel_dp);
> > > > > @@ -1096,6 +1085,28 @@ static void intel_psr_enable_locked(struct
> > > > > intel_dp *intel_dp,
> > > > >         intel_psr_activate(intel_dp);
> > > > >  }
> > > > >  
> > > > > +static void intel_psr_enable_locked(struct intel_dp *intel_dp,
> > > > > +                                   const struct intel_crtc_state
> > > > > *crtc_state,
> > > > > +                                   const struct
> > > > > drm_connector_state
> > > > > *conn_state)
> > > > > +{
> > > > > +       u32 val;
> > > > > +
> > > > > +       intel_dp->psr.psr2_enabled = crtc_state->has_psr2;
> > > > > +       intel_dp->psr.pipe = to_intel_crtc(crtc_state-
> > > > > > uapi.crtc)-
> > > > > > pipe;
> > > > > +       intel_dp->psr.transcoder = crtc_state->cpu_transcoder;
> > > > > +       /* DC5/DC6 requires at least 6 idle frames */
> > > > > +       val =
> > > > > usecs_to_jiffies(intel_get_frame_time_us(crtc_state) *
> > > > > 6);
> > > > > +       intel_dp->psr.dc3co_exit_delay = val;
> > > > > +       intel_dp->psr.psr2_sel_fetch_enabled = crtc_state-
> > > > > > enable_psr2_sel_fetch;
> > > > > +       intel_dp->psr.dc3co_exitline = crtc_state-
> > > > > > dc3co_exitline;
> > > > > +       intel_dp->psr.paused = false;
> > > > > +
> > > > > +       intel_dp_compute_psr_vsc_sdp(intel_dp, crtc_state,
> > > > > conn_state,
> > > > > +                                    &intel_dp->psr.vsc);
> > > > > +
> > > > > +       _intel_psr_enable_locked(intel_dp, crtc_state);
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * intel_psr_enable - Enable PSR
> > > > >   * @intel_dp: Intel DP
> > > > > @@ -1233,6 +1244,60 @@ void intel_psr_disable(struct intel_dp
> > > > > *intel_dp,
> > > > >         cancel_delayed_work_sync(&intel_dp->psr.dc3co_work);
> > > > >  }
> > > > >  
> > > > > +/**
> > > > > + * intel_psr_pause - Pause PSR
> > > > > + * @intel_dp: Intel DP
> > > > > + *
> > > > > + * This function need to be called after enabling psr.
> > > > > + */
> > > > > +void intel_psr_pause(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +       struct intel_psr *psr = &intel_dp->psr;
> > > > > +
> > > > > +       if (!CAN_PSR(intel_dp))
> > > > > +               return;
> > > > > +
> > > > > +       mutex_lock(&psr->lock);
> > > > > +
> > > > > +       if (!psr->active) {
> > > > > +               mutex_unlock(&psr->lock);
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +       intel_psr_exit(intel_dp);
> > > > > +       psr->paused = true;
> > > > > +
> > > > > +       mutex_unlock(&psr->lock);
> > > > > +
> > > > > +       cancel_work_sync(&psr->work);
> > > > > +       cancel_delayed_work_sync(&psr->dc3co_work);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * intel_psr_resume - Resume PSR
> > > > > + * @intel_dp: Intel DP
> > > > > + *
> > > > > + * This function need to be called after pausing psr.
> > > > > + */
> > > > > +void intel_psr_resume(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +       struct intel_psr *psr = &intel_dp->psr;
> > > > > +
> > > > > +       if (!CAN_PSR(intel_dp))
> > > > > +               return;
> > > > > +
> > > > > +       mutex_lock(&psr->lock);
> > > > > +
> > > > > +       if (!psr->paused)
> > > > > +               goto unlock;
> > > > > +
> > > > > +       psr->paused = false;
> > > > > +       intel_psr_activate(intel_dp);
> > > > 
> > > > This patch is doing a bunch of changes around the intel_psr_enable
> > > > but
> > > > then it is calling intel_psr_activate().
> > > > 
> > > > > +
> > > > > +unlock:
> > > > > +       mutex_unlock(&psr->lock);
> > > > > +}
> > > > > +
> > > > >  static void psr_force_hw_tracking_exit(struct intel_dp
> > > > > *intel_dp)
> > > > >  {
> > > > >         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > > index e3db85e97f4c..641521b101c8 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > > @@ -51,5 +51,7 @@ void intel_psr2_program_plane_sel_fetch(struct
> > > > > intel_plane *plane,
> > > > >                                         const struct
> > > > > intel_crtc_state
> > > > > *crtc_state,
> > > > >                                         const struct
> > > > > intel_plane_state *plane_state,
> > > > >                                         int color_plane);
> > > > > +void intel_psr_pause(struct intel_dp *intel_dp);
> > > > > +void intel_psr_resume(struct intel_dp *intel_dp);
> > > > >  
> > > > >  #endif /* __INTEL_PSR_H__ */
> > > > 
> > > 
> > 
> 

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

  reply	other threads:[~2021-05-21 21:52 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-15  3:10 [Intel-gfx] [PATCH v4 00/23] Alder Lake-P Support Matt Roper
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 01/23] drm/i915/xelpd: Enhanced pipe underrun reporting Matt Roper
2021-05-17  6:52   ` Lisovskiy, Stanislav
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 02/23] drm/i915/xelpd: Support DP1.4 compression BPPs Matt Roper
2021-05-17 15:18   ` Jani Nikula
2021-05-18  6:33     ` Kulkarni, Vandita
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 03/23] drm/i915/xelpd: Calculate VDSC RC parameters Matt Roper
2021-05-18 18:06   ` Navare, Manasi
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 04/23] drm/i915/xelpd: Add rc_qp_table for rcparams calculation Matt Roper
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 05/23] drm/i915/xelpd: Add VRR guardband for VRR CTL Matt Roper
2021-05-24 13:40   ` Aditya Swarup
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 06/23] drm/i915/adl_p: Add dedicated SAGV watermarks Matt Roper
2021-05-17  6:49   ` Lisovskiy, Stanislav
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 07/23] drm/i915/adl_p: Setup ports/phys Matt Roper
2021-05-17 18:01   ` Imre Deak
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 08/23] drm/i915/adl_p: Handle TC cold Matt Roper
2021-05-17 14:53   ` Imre Deak
2021-05-17 23:15     ` Souza, Jose
2021-05-17 23:22       ` Souza, Jose
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 09/23] drm/i915/adl_p: Implement TC sequences Matt Roper
2021-05-17 15:12   ` Imre Deak
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 10/23] drm/i915/adl_p: Don't config MBUS and DBUF during display initialization Matt Roper
2021-05-18 11:58   ` Lisovskiy, Stanislav
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 11/23] drm/i915/adl_p: Add ddb allocation support Matt Roper
2021-05-18 12:22   ` Lisovskiy, Stanislav
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 12/23] drm/i915: Introduce MBUS relative dbuf offsets Matt Roper
2021-05-17  6:38   ` Lisovskiy, Stanislav
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 13/23] drm/i915/adl_p: MBUS programming Matt Roper
2023-07-17 10:32   ` Tvrtko Ursulin
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 14/23] drm/i915/adl_p: Tx escape clock with DSI Matt Roper
2021-05-17  7:36   ` Kulkarni, Vandita
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 15/23] drm/i915/display: Replace dc3co_enabled with dc3co_exitline on intel_psr struct Matt Roper
2021-05-17  6:39   ` Gupta, Anshuman
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 16/23] drm/i915/display: Remove a redundant function argument from intel_psr_enable_source() Matt Roper
2021-05-19  6:49   ` Anshuman Gupta
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 17/23] drm/i915/display: Add PSR interrupt error check function Matt Roper
2021-05-17 17:03   ` Souza, Jose
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 18/23] drm/i915/display: Introduce new intel_psr_pause/resume function Matt Roper
2021-05-17 16:58   ` Souza, Jose
2021-05-18  9:33     ` Mun, Gwan-gyeong
2021-05-18 11:06       ` Ville Syrjälä
2021-05-21 10:58         ` Mun, Gwan-gyeong
2021-05-21 21:52           ` Souza, Jose [this message]
2021-06-01 10:23             ` Mun, Gwan-gyeong
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 19/23] drm/i915/adl_p: Define and use ADL-P specific DP translation tables Matt Roper
2021-05-17 23:02   ` Clint Taylor
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 20/23] drm/i915/adl_p: Add PLL Support Matt Roper
2021-05-17 21:55   ` Clint Taylor
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 21/23] drm/i915/adl_p: Program DP/HDMI link rate to DDI_BUF_CTL Matt Roper
2021-05-17 17:01   ` Souza, Jose
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 22/23] drm/i915/adlp: Add PIPE_MISC2 programming Matt Roper
2021-05-17 21:46   ` Clint Taylor
2021-05-15  3:10 ` [Intel-gfx] [PATCH v4 23/23] drm/i915/adl_p: Update memory bandwidth parameters Matt Roper
2021-05-17 17:02   ` Souza, Jose
2021-05-15  4:51 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Alder Lake-P Support (rev3) Patchwork
2021-05-15  4:53 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-05-15  5:22 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-05-17 23:41 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Alder Lake-P Support (rev4) Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c54a7e3d89987a6267b1bf997cd12039926f0282.camel@intel.com \
    --to=jose.souza@intel.com \
    --cc=gwan-gyeong.mun@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=stanislav.lisovskiy@intel.com \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.