On Wed, Jul 12, 2017 at 12:51:10PM +0300, Jani Nikula wrote:
> On Tue, 11 Jul 2017, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> > This old PSR implementation died on gen8LP and had many
> > limitations such:
> > - no functional HW tracking
> > - required link standby with single frame updates
> >
> > So, let's group them in a easier way to filter it now on.
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > Cc: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 5 ++++-
> > drivers/gpu/drm/i915/i915_pci.c | 4 ++--
> > drivers/gpu/drm/i915/intel_dp.c | 2 +-
> > drivers/gpu/drm/i915/intel_psr.c | 7 +++----
> > 4 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 81cd21ecfa7d..d0a49307c6fb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -769,6 +769,7 @@ struct intel_csr {
> > func(has_pipe_cxsr); \
> > func(has_pooled_eu); \
> > func(has_psr); \
> > + func(has_vlv_psr); \
> > func(has_rc6); \
> > func(has_rc6p); \
> > func(has_resource_streamer); \
> > @@ -2983,7 +2984,9 @@ intel_info(const struct drm_i915_private *dev_priv)
> >
> > #define HAS_DDI(dev_priv) ((dev_priv)->info.has_ddi)
> > #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) ((dev_priv)->info.has_fpga_dbg)
> > -#define HAS_PSR(dev_priv) ((dev_priv)->info.has_psr)
> > +#define HAS_PSR(dev_priv) ((dev_priv)->info.has_psr || \
> > + (dev_priv)->info.has_vlv_psr)
> > +#define HAS_VLV_PSR(dev_priv) ((dev_priv)->info.has_vlv_psr)
>
> I'm not convinced adding a intel info field for this is warranted. How
> about just:
>
> #define HAS_VLV_PSR(dev_priv) (HAS_PSR(dev_priv) && \
> (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
>
> Though I'm not convinced about the HS_VLV_PSR name either!
Yeah, for platform-specific stuff like this I'd say make sure you have
vfuncs to split out the difference in behaviour if they're big enough to
warrant an entire macro of its own. And I'd say vlv vs big-core PSR is
different enough to justify having entirely different functions.
-Daniel
>
> BR,
> Jani.
>
>
> > #define HAS_RC6(dev_priv) ((dev_priv)->info.has_rc6)
> > #define HAS_RC6p(dev_priv) ((dev_priv)->info.has_rc6p)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index a1e6b696bcfa..e0e972210090 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -274,7 +274,7 @@ static const struct intel_device_info intel_valleyview_info = {
> > .gen = 7,
> > .is_lp = 1,
> > .num_pipes = 2,
> > - .has_psr = 1,
> > + .has_vlv_psr = 1,
> > .has_runtime_pm = 1,
> > .has_rc6 = 1,
> > .has_gmbus_irq = 1,
> > @@ -334,7 +334,7 @@ static const struct intel_device_info intel_cherryview_info = {
> > .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> > .platform = INTEL_CHERRYVIEW,
> > .has_64bit_reloc = 1,
> > - .has_psr = 1,
> > + .has_vlv_psr = 1,
> > .has_runtime_pm = 1,
> > .has_resource_streamer = 1,
> > .has_rc6 = 1,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2d42d09428c9..3f2191edc2c9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2653,7 +2653,7 @@ static void intel_disable_dp(struct intel_encoder *encoder,
> > if (old_crtc_state->has_audio)
> > intel_audio_codec_disable(encoder);
> >
> > - if (HAS_PSR(dev_priv) && !HAS_DDI(dev_priv))
> > + if (HAS_VLV_PSR(dev_priv))
> > intel_psr_disable(intel_dp);
> >
> > /* Make sure the panel is off before trying to change the mode. But also
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 559f1ab42bfc..461c7e4f6ecc 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -401,8 +401,7 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
> > return false;
> > }
> >
> > - if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> > - !dev_priv->psr.link_standby) {
> > + if (HAS_VLV_PSR(dev_priv) && !dev_priv->psr.link_standby) {
> > DRM_ERROR("PSR condition failed: Link off requested but not supported on this platform\n");
> > return false;
> > }
> > @@ -827,7 +826,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
> > * Single frame update is already supported on BDW+ but it requires
> > * many W/A and it isn't really needed.
> > */
> > - if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> > + if (!HAS_VLV_PSR(dev_priv))
> > return;
> >
> > mutex_lock(&dev_priv->psr.lock);
> > @@ -949,7 +948,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > /* HSW and BDW require workarounds that we don't implement. */
> > dev_priv->psr.link_standby = false;
> > - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > + else if (HAS_VLV_PSR(dev_priv))
> > /* On VLV and CHV only standby mode is supported. */
> > dev_priv->psr.link_standby = true;
> > else
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
