Re: [PATCH v3 8/9] drm/i915/bxt: set chicken bit as IPC y-tile WA

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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@intel.com>
> > > 
> > > 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@intel.com>
> > > ---
> > >   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx