Em Qui, 2016-09-22 às 15:13 +0530, Mahesh Kumar escreveu:
> Hi,
>
>
> On Thursday 22 September 2016 01:53 AM, Paulo Zanoni wrote:
> >
> > Em Sex, 2016-09-09 às 13:31 +0530, Kumar, Mahesh escreveu:
> > >
> > > From: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
> > >
> > > It implements the WA to enable IDLE_WAKEMEM bit of CHICKEN_DCPR_1
> > > register for Broxton platform. When IPC is enabled & Y-tile is
> > > enabled in any of the enabled plane, above bit should be set.
> > > Without this WA system observes random hang.
> > The previous patch enabled the feature, and now this patch
> > implements a
> > missing workaround for the feature. This is not the appropriate
> > order,
> > since it can mean that the previous patch introduced a bug that was
> > fixed by this patch, and this potentially breaks bisectability. We
> > only
> > enable the feature once we know all its workarounds are enabled and
> > the
> > feature will Just Work*. So, normally, my suggestion would be to
> > either
> > invert the patch order, or just make patches 7 and 8 be a single
> > patch.
> I can invert the order of patches, If below point seems OK.
> >
> >
> > But we have another problem, please see
> > commit 590e8ff04bc0182dce97228e5e352d6413d80456. What's the problem
> > with the current implementation? Why can't we just keep the bit as
> > 1
> > forever?
> mentioned commit is making IDLE_WAKEMEM bit always 1b (masking), but
> it
> is require only in case of y/yf tiling.
> need to check for consequences in keeping this bit always Masked.
> what is your opinion on this? Should not we implement it only in case
> of
> y/yf tiling, when we can do it in our framework?
If there are no bad consequences for keeping the bit as 1 even when
we're not using y/yf tiling, then the simpler solution wins IMHO.
>
> Regards,
> -Mahesh
> >
> >
> >
> > >
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > > drivers/gpu/drm/i915/i915_reg.h | 3 +++
> > > drivers/gpu/drm/i915/intel_display.c | 50
> > > ++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4737a0e..79b9305 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1632,6 +1632,8 @@ struct skl_wm_values {
> > > unsigned dirty_pipes;
> > > /* any WaterMark memory workaround Required */
> > > enum watermark_memory_wa mem_wa;
> > > + /* IPC Y-tiled WA related member */
> > > + u32 y_plane_mask;
> > > struct skl_ddb_allocation ddb;
> > > uint32_t wm_linetime[I915_MAX_PIPES];
> > > uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 75b5b52..210d9b0 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5658,6 +5658,9 @@ enum {
> > > #define PLANE_NV12_BUF_CFG(pipe, plane) \
> > > _MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe),
> > > _PLANE_NV12_BUF_CFG_2(pipe))
> > >
> > > +#define CHICKEN_DCPR_1 _MMIO(0x46430)
> > > +#define IDLE_WAKEMEM_MASK (1 << 13)
> > > +
> > > /* SKL new cursor registers */
> > > #define _CUR_BUF_CFG_A 0x7017c
> > > #define _CUR_BUF_CFG_B 0x7117c
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 5f50f53..ee7f88e 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12415,6 +12415,8 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > > bool is_crtc_enabled = crtc_state->active;
> > > bool turn_off, turn_on, visible, was_visible;
> > > struct drm_framebuffer *fb = plane_state->fb;
> > > + struct intel_atomic_state *intel_state =
> > > + to_intel_atomic_state(plane_stat
> > > e-
> > > >
> > > > state);
> > > int ret;
> > >
> > > if (INTEL_GEN(dev) >= 9 && plane->type !=
> > > DRM_PLANE_TYPE_CURSOR) {
> > > @@ -12501,6 +12503,15 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > > !needs_scaling(old_plane_state))
> > > pipe_config->disable_lp_wm = true;
> > >
> > > + if (fb && (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> > > + fb->modifier[0] ==
> > > I915_FORMAT_MOD_Yf_TILED)) {
> > > + intel_state->wm_results.y_plane_mask |=
> > > + (1 <<
> > > drm_plane_index(plane));
> > > + } else {
> > > + intel_state->wm_results.y_plane_mask &=
> > > + ~(1 <<
> > > drm_plane_index(plane));
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -13963,6 +13974,10 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > > if (ret)
> > > return ret;
> > >
> > > + /* Copy the Y-tile WA related states */
> > > + intel_state->wm_results.y_plane_mask =
> > > + dev_priv-
> > > >
> > > > wm.skl_results.y_plane_mask;
> > > +
> > > for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > struct intel_crtc_state *pipe_config =
> > > to_intel_crtc_state(crtc_state);
> > > @@ -14204,6 +14219,32 @@ static void intel_update_crtcs(struct
> > > drm_atomic_state *state,
> > > }
> > > }
> > >
> > > +/*
> > > + * GEN9 IPC WA for Y-tiled
> > > + */
> > > +void bxt_set_ipc_wa(struct drm_i915_private *dev_priv, bool
> > > enable)
> > > +{
> > > + u32 val;
> > > +
> > > + if (!IS_BROXTON(dev_priv) || !i915.enable_ipc)
> > > + return;
> > > +
> > > + val = I915_READ(CHICKEN_DCPR_1);
> > > + /*
> > > + * If WA is already enabled or disabled
> > > + * no need to re-enable or disable it.
> > > + */
> > > + if ((enable && (val & IDLE_WAKEMEM_MASK)) ||
> > > + (!enable && !(val & IDLE_WAKEMEM_MASK)))
> > > + return;
> > > +
> > > + if (enable)
> > > + val |= IDLE_WAKEMEM_MASK;
> > > + else
> > > + val &= ~IDLE_WAKEMEM_MASK;
> > > + I915_WRITE(CHICKEN_DCPR_1, val);
> > > +}
> > > +
> > > static void skl_update_crtcs(struct drm_atomic_state *state,
> > > unsigned int *crtc_vblank_mask)
> > > {
> > > @@ -14219,6 +14260,12 @@ static void skl_update_crtcs(struct
> > > drm_atomic_state *state,
> > > enum pipe pipe;
> > >
> > > /*
> > > + * If IPC WA need to be enabled, enable it now
> > > + */
> > > + if (intel_state->wm_results.y_plane_mask)
> > > + bxt_set_ipc_wa(dev_priv, true);
> > > +
> > > + /*
> > > * Whenever the number of active pipes changes, we need
> > > to
> > > make sure we
> > > * update the pipes in the right order so that their
> > > ddb
> > > allocations
> > > * never overlap with eachother inbetween CRTC updates.
> > > Otherwise we'll
> > > @@ -14261,6 +14308,9 @@ static void skl_update_crtcs(struct
> > > drm_atomic_state *state,
> > > progress = true;
> > > }
> > > } while (progress);
> > > +
> > > + if (!intel_state->wm_results.y_plane_mask)
> > > + bxt_set_ipc_wa(dev_priv, false);
> > > }
> > >
> > > static void intel_atomic_commit_tail(struct drm_atomic_state
> > > *state)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
