Re: [PATCH] drm/i915/hsw: enable atomic in L3 for some steppings.

[Francisco Jerez <currojerez@riseup.net>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4272 bytes --]

Zhigang Gong <zhigang.gong@linux.intel.com> writes:

> On Mon, Jan 05, 2015 at 05:03:16AM +0200, Francisco Jerez wrote:
>> Zhigang Gong <zhigang.gong@intel.com> writes:
>> 
>> > According to bspec, ROW_CHICKEN3's bit 6 which is to disable
>> > move of cacheable global atomics to L3 is needed for GT3 D
>> > stepping.
>> >
>> > I enabled it and tested it with HSW GT2 D stepping and GT3 E stepping.
>> > The atomics works fine in beignet. And it could get more than 10x performance
>> > improvement with some workload, for an example, the "splat" kernel in darktable,
>> > without this patch, it consumes 50 seconds in one large raw picture processing.
>> > But with this patch, the same process only takes less than 1 second.
>> >
>> 
>> I tried this already (on HSW GT2 D as well) and I don't think it's
>> enough to get L3 atomics working reliably.  Even though they did seem to
>> work OK at first glance I observed some corruption issues (e.g. atomic
>> writes not landing in system memory) when doing atomic writes to
>> contiguous (as in within the same cache-line) locations in memory.  The
>> "unused" ARB_shader_image_load_store test [1] I sent to the Piglit
>> mailing list some time ago exposes this IIRC, and probably a couple of
>> other tests too.
> Ok, I will find that case and have a try on my systems. I just tested all
> the atomic related OpenCL conformance test cases without any issues.
>
>> 
>> Also this change is going to cause an instant lock-up anytime Mesa uses
>> atomics because Mesa doesn't change the default L3 way allocation for
>> the DC, which turns out to be 0 on HSW.
>
> This is another issue, IMHO, if the application wants to use atomics,
> it's better to allocate some L3 space for DC. Otherwise, it could
> never leverage the "atomics in L3 feature". Based on my test,
> the performance impact as huge as more than 10x for some workloads.
>
Sure, but this change alone will cause a regression (an irrecoverable
system hang) with current releases of Mesa. 

I agree that Mesa should be fixed eventually to assign L3 space to the
DC for some workloads, but it doesn't seem like we have a satisfactory
API to do that right now -- The current mechanism used by Beignet
(poking the L3 control registers from the batch buffer) is slightly
concerning from a security point of view because it allows an arbitrary
userspace application to cause misrendering or impact the performance of
other clients (since the L3 config registers are not being saved and
restored as part of the context) and even crash the whole system.  It
also doesn't look like it could be made to work with the hardware
command checker.

> Thanks,
> Zhigang Gong. 
>
>> 
>> [1] http://lists.freedesktop.org/archives/piglit/2014-December/013571.html
>> 
>> > Signed-off-by: Zhigang Gong <zhigang.gong@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++----
>> >  1 file changed, 6 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > index 7d99a9c..8a27802 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -5938,10 +5938,12 @@ static void haswell_init_clock_gating(struct drm_device *dev)
>> >  
>> >  	ilk_init_lp_watermarks(dev);
>> >  
>> > -	/* L3 caching of data atomics doesn't work -- disable it. */
>> > -	I915_WRITE(HSW_SCRATCH1, HSW_SCRATCH1_L3_DATA_ATOMICS_DISABLE);
>> > -	I915_WRITE(HSW_ROW_CHICKEN3,
>> > -		   _MASKED_BIT_ENABLE(HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE));
>> > +	if (IS_HSW_GT3(dev) && dev->pdev->revision <= 6) {
>> > +		/* L3 caching of data atomics doesn't work -- disable it. */
>> > +		I915_WRITE(HSW_SCRATCH1, HSW_SCRATCH1_L3_DATA_ATOMICS_DISABLE);
>> > +		I915_WRITE(HSW_ROW_CHICKEN3,
>> > +			   _MASKED_BIT_ENABLE(HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE));
>> > +	}
>> >  
>> >  	/* This is required by WaCatErrorRejectionIssue:hsw */
>> >  	I915_WRITE(GEN7_SQ_CHICKEN_MBCUNIT_CONFIG,
>> > -- 
>> > 1.8.3.2
>
>
>
>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx