On 12/7/15, 11:56 AM, "Deak, Imre" <imre.deak@xxxxxxxxx> wrote:
>On Mon, 2015-12-07 at 21:28 +0200, Ville Syrjälä wrote:
>> On Mon, Dec 07, 2015 at 10:51:09AM -0800, Wayne Boyer wrote:
>> > Do some further clean up based on the initial review of
>> > drm/i915: Separate cherryview from valleyview.
>> >
>> > In this case, in i915_gem_alloc_context_obj() only call
>> > i915_gem_object_set_cache_level() for Ivy Bridge devices
>> > since later platforms don't have L3 control bits in the PTE.
>> >
>> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> > Signed-off-by: Wayne Boyer <wayne.boyer@xxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/i915_gem_context.c | 8 +++-----
>> > 1 file changed, 3 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
>> > b/drivers/gpu/drm/i915/i915_gem_context.c
>> > index 4b1161d..e4de433 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> > @@ -185,12 +185,10 @@ i915_gem_alloc_context_obj(struct drm_device
>> > *dev, size_t size)
>> > /*
>> > * Try to make the context utilize L3 as well as LLC.
>> > *
>> > - * On VLV we don't have L3 controls in the PTEs so we
>> > - * shouldn't touch the cache level, especially as that
>> > - * would make the object snooped which might have a
>> > - * negative performance impact.
>> > + * This is only applicable for Ivy Bridge devices since
>> > + * later platforms don't have L3 control bits in the PTE.
>> > */
>> > - if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) &&
>> > !IS_CHERRYVIEW(dev)) {
>> > + if (IS_IVYBRIDGE(dev)) {
>>
>> This would actually change the snoop setting on BXT, but that
>> should be fine since BXT still has the GTT -> PAT 0 thing which
>> means we get snooping anyway IIRC.
>>
>> Imre, we discussed this stuff at some point, and I hope I'm not
>> forgetting something crucial here that would break BXT. Probably best
>> if you sanity check my thinking here...
>
>Yes, AFAIU it doesn't matter what level we set, because of the PAT
>mapping constraint you describe above. Atm we also don't use this
>function on BXT, b/c of the different codepaths for LRC vs. ringbuffer
>mode. But that could/should be unified at one point, so yea it's better
>to keep things working for BXT too here. In the corresponding
>intel_lr_context_deferred_alloc() we don't set the cache level, which
>also agrees with the above, so I think this change is fine.
>
>> We should probably mention this PAT 0 business in a comment somewhere
>> here since it's a very easy thing to miss.
>
>Yep, a comment about this would be in order.
Ville, Imre, how does this look for an updated comment?
/*
* Try to make the context utilize L3 as well as LLC.
*
* On VLV we don't have L3 controls in the PTE so we
* shouldn't touch the cache level, especially as that
* would make the object snooped which might have a
* negative performance impact.
*
* On CHV and BXT we set PPAT 0 in chv_setup_private_ppat()
* which means we get snooping anyway.
*
* This is only applicable for Ivy Bridge devices since
* later platforms don't have L3 control bits in the PTE.
*/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
