Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Aug 22, 2018 at 06:32:33PM +0530, Kumar, Mahesh wrote:
> Hi,
> 
> 
> On 8/22/2018 12:26 AM, Rodrigo Vivi wrote:
> > On Tue, Aug 21, 2018 at 09:30:21PM +0530, Kumar, Mahesh wrote:
> > > Hi,
> > > 
> > > 
> > > On 8/21/2018 8:27 PM, Kumar, Mahesh wrote:
> > > > Hi,
> > > > 
> > > > 
> > > > On 8/17/2018 11:50 PM, Rodrigo Vivi wrote:
> > > > > On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
> > > > > > IPC may cause underflows if not used with dual channel symmetric
> > > > > > memory configuration. Disable IPC for non symmetric configurations in
> > > > > > affected platforms.
> > > > > > Display WA #1141
> > > > > > 
> > > > > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/i915_drv.c | 43
> > > > > > ++++++++++++++++++++++++++++++++++++-----
> > > > > >    drivers/gpu/drm/i915/i915_drv.h |  1 +
> > > > > >    drivers/gpu/drm/i915/intel_pm.c |  2 +-
> > > > > >    3 files changed, 40 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > index 86bc2e685522..2273664166bc 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct
> > > > > > dram_channel_info *ch, u32 val)
> > > > > >        return 0;
> > > > > >    }
> > > > > > 
> > > > > > +static bool
> > > > > > +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
> > > > > > +              u32 val_ch0, u32 val_ch1,
> > > > > > +              struct dram_channel_info *ch0)
> > > > > what about
> > > > > intel_is_dram_symmetric() ?
> > > > sounds good.
> > > > > > +{
> > > > > > +    /* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */
> > > > > move this to the wa call, not the the function check...
> > > > > 
> > > > > > +    if (INTEL_GEN(dev_priv) > 9 &&
> > > > > > +        !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
> > > > > please don't add CNL pre-prod wa
> > > > ok sure
> > > > > > +        return true;
> > > > > > +
> > > > > > +    if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
> > > > > > +        return true;
> > > > > actually remove all platforms checks here...
> > > > Agree will remove these checks, function will just return if memory is
> > > > symmetric.
> > > > > > +
> > > > > > +    if (val_ch0 != val_ch1)
> > > > > > +        return false;
> > > > > > +
> > > > > > +    if (ch0->s_info.size == 0)
> > > > > > +        return true;
> > > > > > +    if (ch0->l_info.size == ch0->s_info.size &&
> > > > > > +        ch0->l_info.width == ch0->s_info.width &&
> > > > > > +        ch0->l_info.rank == ch0->s_info.rank)
> > > > > > +        return true;
> > > > > > +
> > > > > > +    return false;
> > > > > return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
> > > > >          (ch0->l_info.size == ch0->s_info.size &&
> > > > >            ch0->l_info.width == ch0->s_info.width &&
> > > > >            ch0->l_info.rank == ch0->s_info.rank)))
> > > > > 
> > > > > > +}
> > > > > > +
> > > > > >    static int
> > > > > >    skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
> > > > > >    {
> > > > > >        struct dram_info *dram_info = &dev_priv->dram_info;
> > > > > >        struct dram_channel_info ch0, ch1;
> > > > > > -    u32 val;
> > > > > > +    u32 val_ch0, val_ch1;
> > > > > >        int ret;
> > > > > > 
> > > > > > -    val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > > > > > -    ret = skl_dram_get_channel_info(&ch0, val);
> > > > > > +    val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > > > > > +    ret = skl_dram_get_channel_info(&ch0, val_ch0);
> > > > > >        if (ret == 0)
> > > > > >            dram_info->num_channels++;
> > > > > > 
> > > > > > -    val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > > > > > -    ret = skl_dram_get_channel_info(&ch1, val);
> > > > > > +    val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > > > > > +    ret = skl_dram_get_channel_info(&ch1, val_ch1);
> > > > > >        if (ret == 0)
> > > > > >            dram_info->num_channels++;
> > > > > > 
> > > > > > @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > >        if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
> > > > > >            dram_info->is_16gb_dimm = true;
> > > > > > 
> > > > > > +    if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
> > > > > > +        dev_priv->ipc_capable_mem = true;
> > > > > > +    else
> > > > > > +        dev_priv->ipc_capable_mem = false;
> > > > > > +
> > > > > > +    DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
> > > > > > +              dev_priv->ipc_capable_mem ? "" : "not ");
> > > > > >        return 0;
> > > > > >    }
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 854f3c828e01..036d6554c017 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -2112,6 +2112,7 @@ struct drm_i915_private {
> > > > > >        bool chv_phy_assert[2];
> > > > > > 
> > > > > >        bool ipc_enabled;
> > > > > > +    bool ipc_capable_mem;
> > > > > I don't think we need to stage this...
> > > > With above changes storing this info is not needed, But we need to keep
> > > > a flag in dram_info to check if "memory is symmetric", Otherwise we need
> > > > to compute all the memory related info again.
> > > > 
> > > > -Mahesh
> > > > > >        /* Used to save the pipe-to-encoder mapping for audio */
> > > > > >        struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 2446f53adf21..39e400d5f555 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > >        u32 val;
> > > > > > 
> > > > > >        /* Display WA #0477 WaDisableIPC: skl */
> > > > > > -    if (IS_SKYLAKE(dev_priv)) {
> > > > > > +    if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
> > > > > >            dev_priv->ipc_enabled = false;
> > > > > This is not the WA 1141 for other platforms than SKL. Please only
> > > > > keep skl here.
> > > > > 
> > > > > For the other WA add 4us across all watermark levels
> > > > > 
> > > > > /* Display WA #1141: skl,kbl,cfl */
> > > > > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
> > > > >      intel_is_dram_symmetric())
> > > > >      levels += 4;
> > > Adding 4us in latency is already taken care in skl_compute_plane_wm.
> > > Will handle here, enabling of IPC only if memory is symmetric.
> > But WA1141 describe that we only need to avoid IPC on SKL...
> > not on every gen9... Everywhere else the 4s should be enough.
> > 
> > so if the 4us is already in place maybe the patch that we need
> > is to only apply the 4us if memory is symmetric and not skl.
> WA also says enable IPC only if memory is symmetric,
> <KBL WA: When IPC is enabled, watermark latency values must be increased by
> 4us across all levels.
> IPC must be enabled only with dual channel symmetric memory configurations.>

Oh! Indeed. I'm sorry..

so we need something like:

/* Display WA #0477 WaDisableIPC: skl */
/* Display WA #1141: skl,kbl,cfl */
 if (IS_SKYLAKE(dev_priv) ||
    ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
      !intel_is_dram_symmetric())
     	  dev_priv->ipc_enabled = false;

> 
> -Mahesh
> > 
> > > -Mahesh
> > > > > >            return;
> > > > > >        }
> > > > > > -- 
> > > > > > 2.16.2
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx