On Mon, Dec 07, 2015 at 10:26:15PM +0000, Boyer, Wayne wrote:
> 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.
Maybe something like this:
"Snooping is required on non-llc platforms in execlist
mode, but since all GGTT accesses use PAT entry 0 we
get snooping anyway regardless of cache_level.
Otherwise lgtm.
> *
> * This is only applicable for Ivy Bridge devices since
> * later platforms don't have L3 control bits in the PTE.
> */
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
