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

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

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.

> 
> -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