Re: [PATCH 04/12] drm/i915/gen9: Merge HALF_SLICE_CHICKEN3 WA

From: Daniel Vetter <daniel@ffwll.ch>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, Mika Kuoppala <mika.kuoppala@intel.com>
Subject: Re: [PATCH 04/12] drm/i915/gen9: Merge HALF_SLICE_CHICKEN3 WA
Date: Mon, 28 Sep 2015 15:56:56 +0200	[thread overview]
Message-ID: <20150928135656.GQ3383@phenom.ffwll.local> (raw)
In-Reply-To: <87k2rbkk1h.fsf@intel.com>

On Mon, Sep 28, 2015 at 11:47:06AM +0300, Jani Nikula wrote:
> On Fri, 25 Sep 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Sep 25, 2015 at 02:33:38PM +0100, Arun Siluvery wrote:
> >> Merge WaDisableSamplerPowerBypassForSOPingPong and another WA which has no name
> >> as they are part of same register. This will save an entry in WA array.
> >> 
> >> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++----------
> >>  1 file changed, 8 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index ad16ef4..963b3ca 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -914,9 +914,14 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
> >>  	WA_SET_BIT_MASKED(GEN8_ROW_CHICKEN,
> >>  			  PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE);
> >>  
> >> -	/* Syncing dependencies between camera and graphics:skl,bxt */
> >> -	WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN3,
> >> -			  GEN9_DISABLE_OCL_OOB_SUPPRESS_LOGIC);
> >> +	/* WA: Syncing dependencies between camera and graphics:skl,bxt */
> >> +	/* WaDisableSamplerPowerBypassForSOPingPong:skl,bxt */
> >> +	tmp = GEN9_DISABLE_OCL_OOB_SUPPRESS_LOGIC;
> >> +	if (IS_SKYLAKE(dev) ||
> >> +	    (IS_BROXTON(dev) && INTEL_REVID(dev) <= BXT_REVID_B0)) {
> >> +		tmp |= GEN8_SAMPLER_POWER_BYPASS_DIS;
> >> +	}
> >> +	WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN3, tmp);
> >
> > I really dislike these platform+stepping checks in the shared codepath.
> > If there's any difference between the platforms, IMO the w/a should go into
> > the per-platform function.
> 
> Agreed, also on a more generic level than these specific functions.

Concurred, sharing code just to reduce the line count is a bad idea, code
should only be shared if it actually does the same thing. Better to be
verbose than accidentally break things.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx